Loop Refactor 1/N - Training Loop #7871

awaelchli · 2021-06-07T20:42:15Z

What does this PR do?

Part of #7700

Introduces the new loop interface.
Old classes will be removed asap in a follow up.

How to review this PR

Review the new loop base class found in pytorch_lightning/loops/base.py
Review the new training loop classes
- EpochLoop: runs over epochs
- TrainingLoop: runs over all batches in a single epoch
- BatchLoop: runs over a single batch (could include looping over several optimizers or truncated backprop steps).
Review changes in the Trainer class file. Here we have some re-routing from old to new. This will be cleaned up

The new training loop classes are re-organizing the old TrainLoop class.
Important: Changes requested on code lines that are 1-1 from the old TrainLoop class will be dismissed. We cannot address these comments here. More refactor iterations will be required to simplify all existing helper functions.

TODO:

typing (@justusschock wants to do it)
docstrings (@justusschock wants to do it)
[x[ get all tests to pass

More follow-up work is listed in this issue tracker: #7938

Before submitting

Was this discussed/approved via a GitHub issue? (not for typos and docs)
Did you read the contributor guideline, Pull Request section?
Did you make sure your PR does only one thing, instead of bundling different changes together?
Did you make sure to update the documentation with your changes? (if necessary)
Did you write any new necessary tests? (not for typos and docs)
Did you verify new and existing tests pass locally with your changes?
Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

Is this pull request ready for review? (if not, please submit in draft mode)
Check that all items from Before submitting are resolved
Make sure the title is self-explanatory and the description concisely explains the PR
Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

Make sure you had fun coding 🙃

Fixes #7034

Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com> Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

…train' into refactor/loops/loops_everywhere_train

pytorch_lightning/loops/base.py

kaushikb11

What a kickass PR!

for more information, see https://pre-commit.ci

…train' into refactor/loops/loops_everywhere_train

tchaton · 2021-06-14T19:08:19Z

Hey @awaelchli,

It is not a blocker for this PR, but there is a small inconsistency in the loop right now.
"on_train_epoch_start" is being called in FitLoop `on_advance_start
and "on_train_epoch_end" is being called in TrainellopLoop in "on_run_end".

Best,
T.C

awaelchli · 2021-06-14T19:46:56Z

great observation @tchaton, moved to on_run_start!

CHANGELOG.md

pytorch_lightning/loops/fit_loop.py

pytorch_lightning/loops/base.py

pytorch_lightning/trainer/connectors/logger_connector/logger_connector.py

Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>

carmocca · 2021-09-03T00:17:05Z

pytorch_lightning/loops/fit_loop.py

+            if epoch_output is None:
+                return


@awaelchli do you remember why this was necessary?

edit: checking here #9298

there is a good reason, something along those lines of my other comment. pretty sure without this a test will fail. I would not have added it otherwise. But I should have added a comment. De-cyphering handling of these edge cases was definitely a center point of frustration for these loop refactors.

pretty sure this: https://pytorch-lightning.readthedocs.io/en/latest/common/lightning_module.html?highlight=skip%20epoch#on-train-batch-start

returning -1 here skipping the epoch

carmocca · 2021-09-03T00:18:28Z

pytorch_lightning/loops/training_epoch_loop.py

+        if self.batches_seen == 0:
+            # dataloader/iterator did not produce a batch
+            return


@awaelchli do you remember why this was necessary?

edit: checking here #9298

yes, basically if the epoch is empty we don't want to run epoch_end hooks or invoke any logging. when does this happen? a user can stop training or skip an epoch by returning a signal from one of the hooks. I don't remember the exact use case. Pretty sure if you remove this line a test will fail and inform you exactly the reason.

carmocca and others added 30 commits May 31, 2021 20:44

forked_name

5ca5b97

is_tensor_and

0a6b185

Add missing variable

30eff02

Refactor fwd

e4c7d2f

Merge branch 'master' into refactor/logger-connector-poc

17cd9d1

Merge branch 'master' into refactor/loops/loops_everywhere

b2de630

integrate #7772

a9adba0

Forgot to pass value

3881b18

Docstrings

041ec8e

Revert if condition

1047836

is_train -> training

b33f5dc

Unnecessary getter/setter

43777a6

Unnecessary getter/setter

b7b9633

Linter

5768b55

root_device -> device

08fac3d

Docstrings

7886151

Better exceptions

be94ec9

flake8 and mypy

76eb0ee

Clean up batch size?

ae93240

Replace repr with str

8fbbcff

Refactor reset

df0ff74

Docstring

20bcf0c

hook_name -> fx

6da9d8f

Fix reset

f10084e

Refactor

13785a0

Serialization updates

cce7023

Merge branch 'master' into refactor/logger-connector-poc

54da8eb

Move is_tensor to ResultMetric

f51a2aa

Fixes

5700df4

Rename metric attribute

bf8bc52

justusschock and others added 3 commits June 14, 2021 17:45

Apply suggestions from code review

bf26aa3

Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com> Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

Apply suggestions from code review

ffc4f45

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

Merge remote-tracking branch 'origin/refactor/loops/loops_everywhere_…

79f8c18

…train' into refactor/loops/loops_everywhere_train

kaushikb11 reviewed Jun 14, 2021

View reviewed changes

pytorch_lightning/loops/base.py Show resolved Hide resolved

kaushikb11 approved these changes Jun 14, 2021

View reviewed changes

awaelchli and others added 5 commits June 14, 2021 18:28

resolve a todo integrating on_train_batch_end with on_advance_end

3373cc8

clarify what is todo and what is fixme

e1a40c0

[pre-commit.ci] auto fixes from pre-commit.com hooks

b5bb08a

for more information, see https://pre-commit.ci

shorten a docstring

5d98009

Merge remote-tracking branch 'origin/refactor/loops/loops_everywhere_…

03bce7a

…train' into refactor/loops/loops_everywhere_train

awaelchli added 2 commits June 14, 2021 21:28

move on_epoch_start to on_run_start of training loop

f001f81

Merge branch 'master' into refactor/loops/loops_everywhere_train

24fa859

carmocca reviewed Jun 15, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

pytorch_lightning/loops/fit_loop.py Show resolved Hide resolved

pytorch_lightning/loops/base.py Outdated Show resolved Hide resolved

pytorch_lightning/trainer/connectors/logger_connector/logger_connector.py Show resolved Hide resolved

awaelchli and others added 4 commits June 15, 2021 03:00

Update pytorch_lightning/loops/base.py

d191fe1

Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>

update class names in changelog

1d21065

add empty teardown method

2d8c441

added skip property

f874182

tchaton mentioned this pull request Jun 15, 2021

[feat] Add tracking on new TrainLoop #7976

Closed

11 tasks

awaelchli enabled auto-merge (squash) June 15, 2021 12:00

awaelchli mentioned this pull request Jun 15, 2021

Loop Refactor 2/N - Remove Old Training Loop #7985

Merged

11 tasks

awaelchli merged commit 971908a into master Jun 15, 2021

awaelchli deleted the refactor/loops/loops_everywhere_train branch June 15, 2021 12:55

This was referenced Jun 15, 2021

Loop Refactor 3/N - Evaluation Loop #7990

Merged

Loop Refactor 5/N - Prediction Loop #7700

Merged

carmocca mentioned this pull request Jun 21, 2021

Update fit with val hook test #8060

Merged

9 tasks

carmocca reviewed Sep 3, 2021

View reviewed changes

mergify bot added the ready PRs ready to be merged label Sep 3, 2021

awaelchli mentioned this pull request Sep 3, 2021

Remove unnecessary TrainingEpochLoop return #9298

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loop Refactor 1/N - Training Loop #7871

Loop Refactor 1/N - Training Loop #7871

awaelchli commented Jun 7, 2021 •

edited

Loading

kaushikb11 left a comment

tchaton commented Jun 14, 2021

awaelchli commented Jun 14, 2021

carmocca Sep 3, 2021 •

edited

Loading

awaelchli Sep 3, 2021

awaelchli Sep 3, 2021

carmocca Sep 3, 2021 •

edited

Loading

awaelchli Sep 3, 2021

Loop Refactor 1/N - Training Loop #7871

Loop Refactor 1/N - Training Loop #7871

Conversation

awaelchli commented Jun 7, 2021 • edited Loading

What does this PR do?

How to review this PR

Before submitting

PR review

Did you have fun?

kaushikb11 left a comment

Choose a reason for hiding this comment

tchaton commented Jun 14, 2021

awaelchli commented Jun 14, 2021

carmocca Sep 3, 2021 • edited Loading

Choose a reason for hiding this comment

awaelchli Sep 3, 2021

Choose a reason for hiding this comment

awaelchli Sep 3, 2021

Choose a reason for hiding this comment

carmocca Sep 3, 2021 • edited Loading

Choose a reason for hiding this comment

awaelchli Sep 3, 2021

Choose a reason for hiding this comment

awaelchli commented Jun 7, 2021 •

edited

Loading

carmocca Sep 3, 2021 •

edited

Loading

carmocca Sep 3, 2021 •

edited

Loading